Skip to content

Conversation

@ocavue
Copy link
Contributor

@ocavue ocavue commented Dec 7, 2024

npm run typegen (and npm run build) now will check TypeScript errors.

I migrated jsconfig.json to tsconfig.json because tsc --build can only recognize tsconfig.json. tsc --build is faster than tsc because tsc --build has cache, so it's better to use tsc --build.


Since it reveals all typing errors now, I need to address all of them to make the build pass.

  • Most of them are just missing types in configs, like the one below:

    this.config.image_token_index;
                ~~~~~~~~~~~~~~~~~
                ^ Property 'image_token_index' does not exist on type 'PretrainedConfig'.

    For those errors, I simply add @ts-expect-error comment to let TypeScript ignore it. I use @ts-expect-error instead of @ts-ignore because we can remove those comments once we add missing types in the future.

  • I fixed some other types issues that I can fix.

  • One thing I noticed is that MgpstrProcessor.batch_decode has different shape compared to the method in the parent class. This might not be a good idea.

@ocavue ocavue marked this pull request as ready for review December 7, 2024 23:37
@xenova
Copy link
Collaborator

xenova commented Dec 8, 2024

Thanks for this! I'll do a full review soon, but just to address your third point: https://github.com/huggingface/transformers/blob/c8c8dffbe45ebef0a8dba4a51024e5e5e498596b/src/transformers/models/mgp_str/processing_mgp_str.py#L102 has a similar issue where MgpstrProcessor has a non-standard batch_decode function. This is because the model returns three tensors that all need to be used in the decode method. Not too sure how to make it fit the parent class type though.

@ocavue
Copy link
Contributor Author

ocavue commented Dec 8, 2024

I would consider this as an issue in the original Python transformers lib. At least its docstring is wrong.

I don't know any context about MgpstrProcessor, but maybe renaming this method to something else (e.g. just decode()) could improve it.


On the JS lib side, it's okey for me to ignore it for now. Nobody except TypeScript complains so far.

@xenova
Copy link
Collaborator

xenova commented Dec 28, 2024

Apologies for the delay - will finish the review now 👍

Copy link
Collaborator

@xenova xenova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! Thanks again 😇

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@xenova xenova merged commit c845bb5 into huggingface:main Dec 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants